Render stroke via List<Graphic>#4160
Conversation
# Conflicts: # node-graph/libraries/rendering/src/renderer.rs
This simplifies the future implementation of clipping-based rendering for strokes, as the stroke does not support the use of a clip path but rather paint sources from a paint server.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
This exposes List's attributes to message handlers, enabling them to access the necessary attribute data such as ATTR_STROKE_PAINT_GRAPHIC as `Fill` and `Stroke` will not have paint information in the future.
YohYamasaki
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the vector data and rendering pipeline to support List<Graphic> for fill and stroke attributes, moving away from the fixed Fill and Stroke enums. It introduces new attributes ATTR_FILL_GRAPHIC and ATTR_STROKE_PAINT_GRAPHIC, updates the RenderExt trait to handle fill and stroke targets separately, and implements pattern-based fills in SVG. Review feedback highlights issues with hardcoded "fill" attributes in fallback cases, potential invalid SVG IDs for empty gradients, and a missing transparency check in the Vello renderer.
| _render_params: &RenderParams, | ||
| target: PaintTarget, | ||
| ) -> Self::Output { | ||
| let Some(color) = self.element(0) else { return r#" fill="none""#.to_string() }; |
There was a problem hiding this comment.
The fallback value for an empty List hardcodes the fill attribute. This will produce invalid SVG attributes when the PaintTarget is Stroke (e.g., stroke="#..." fill="none"). It should use target.paint_attr() to correctly return stroke="none" when applicable.
| let Some(color) = self.element(0) else { return r#" fill="none""#.to_string() }; | |
| let Some(color) = self.element(0) else { return format!(r#" {}="none""#, target.paint_attr()) }; |
| let paint_attr = target.paint_attr(); | ||
|
|
||
| match fill_graphic { | ||
| Some(Graphic::Color(color_list)) => color_list.render(svg_defs, item_transform, element_transform, stroke_transform, bounds, transformed_bounds, render_params, target), |
There was a problem hiding this comment.
Similar to the issue in impl RenderExt for List, the fallback for an empty color list here should respect the PaintTarget instead of hardcoding fill="none".
| let gradient_id = gradient_list.render(svg_defs, item_transform, element_transform, stroke_transform, bounds, transformed_bounds, render_params, target); | ||
| format!(r##" {paint_attr}="url(#{gradient_id})""##) |
There was a problem hiding this comment.
If gradient_list is empty, render returns 0, resulting in url(#0). This is likely invalid SVG as no element with id="0" exists. This case should return none for the paint attribute.
| // FIXME: Need to add Graphic.is_fully_transparent check | ||
| let can_draw_aligned_stroke = stroke.as_ref().is_some_and(|s| s.has_renderable_stroke() && s.align.is_not_centered()) && element.stroke_bezier_paths().all(|p| p.closed()); |
There was a problem hiding this comment.
The FIXME note regarding is_fully_transparent should be addressed to ensure that aligned strokes are correctly skipped when the paint is invisible, matching the logic added to the SVG renderer.
|
I will combine this PR into #4111 as it requires changes related to fill. |
1 participant
WIP
Use
List<Graphic>inATTR_STROKE_PAINT_GRAPHICfor the paint of a stroke instead ofStroke.color.This PR depends on #4111, thus it needs to be merged first.